-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
16409 do not use receive step to determine root report #16723
16409 do not use receive step to determine root report #16723
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Test Results1 258 tests +2 1 254 ✅ +2 7m 27s ⏱️ -10s Results for commit 5e8a3c3. ± Comparison against base commit 986d6fb. This pull request removes 2 and adds 4 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
5c35630
to
4f56b4b
Compare
@@ -178,7 +180,8 @@ class FHIRConverter( | |||
// is properly recorded in the report file table with the correct sender | |||
actionHistory.trackExternalInputReport( | |||
report, | |||
BlobAccess.BlobInfo(format, blobUrl, blobDigest.toByteArray()) | |||
BlobAccess.BlobInfo(format, blobUrl, blobDigest.toByteArray()), | |||
payloadName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change to carry over payloadname
from the submissions API is needed to continue supporting the sendOriginal
function. This adds the information to the root report being created here.
.leftJoin(REPORT_LINEAGE) | ||
.on(REPORT_FILE.REPORT_ID.eq(REPORT_LINEAGE.CHILD_REPORT_ID)) | ||
.where(REPORT_LINEAGE.PARENT_REPORT_ID.isNull()) | ||
.orderBy(REPORT_FILE.ACTION_ID.asc()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous query was to join the action
table in order to check if the action_name
is receive
. The new query instead filters the list of reports to only include those that do not appear in report_lineage
as a child. This list is then ordered by action_id
to ensure the results are output in a consistent order. The join to the action
table is removed since it is no longer necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this explanation was very helpful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, we are now recursively going up the report lineage table until we find a parent report id which is null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recursive query (cte
) was not altered, but that is the current and previous behavior. The change here was in the way the results from the recursive query are filtered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it 👍
@@ -798,4 +798,166 @@ class FHIRTranslatorIntegrationTests : Logging { | |||
assertThat(translatedValue).isEqualTo(reportContents.toByteArray()) | |||
} | |||
} | |||
|
|||
@Test | |||
fun `successfully translate HL7 for FHIR receiver when isSendOriginal is true from convert step`() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests ensure sendOriginal
items still function when convert
is the root report, as the case would be when the submissions API is used.
d665d9b
to
b7c1982
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to the best of my understanding.
I ran the tests locally and everything is as expected.
Side note: I wanted to use this PR as an excuse to try out the new submission microservice. I was not able to get any reports sent to it because of setup issues. Most likely this is because of the extra steps currently required to get the authorization service running locally. This should no block this PR.
@@ -111,6 +111,7 @@ class FHIRConverter( | |||
companion object { | |||
|
|||
private val clientIdHeader = "client_id" | |||
private val payloadNameHeader = "payloadname" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be done now, but since in the future these header names will be used across multiple services they should be defined in the shared project. It makes it less prone to errors if we have to change them for some reason in the future.
Maybe you could throw a TODO
on there with your ticket for submissions service updates.
b7c1982
to
5e8a3c3
Compare
Quality Gate passedIssues Measures |
This reverts commit 6cb62ec.
This reverts commit 6cb62ec.
This PR adjusts the search for root reports to not rely on explicitly searching for the
receive
step report. Additionally, the root report from the submissions endpoint should now include the sender-suppliedpayloadname
if present.Test Steps:
ReportGraphTest
andFhirTranslatorIntegrationTests
are sufficientChanges
receive
step as indicator of a root report.report_lineage
table as a child report.payloadname
from submission endpoint message headers when creating root report.Checklist
Testing
./prime test
or./gradlew testSmoke
against local Docker ReportStream container?npm run lint:write
?Linked Issues